-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a CSR batched matrix format, CUDA, HIP and DPCPP kernels #1450
Conversation
5ef3670
to
cf08426
Compare
f600023
to
79e68b3
Compare
cf08426
to
e694784
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is pretty late, so I only went over the implementation, not the tests. Most of it LGTM, there is just a behavioral change in here that I believe overshoots in fixing a specific edge case.
@@ -37,7 +37,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||
|
|||
|
|||
#include <ginkgo/core/base/batch_multi_vector.hpp> | |||
#include <ginkgo/core/matrix/batch_dense.hpp> | |||
#include <ginkgo/core/matrix/batch_ell.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover?
@upsj , this was not meant to be in the release. I had a bit of time, so decided to work on it. Thanks for reviewing, but this is still a WIP, that is why I havent requested any reviews or marked it ready-for-review yet. |
@pratikvn phew, thank you, that would have been a night shift otherwise 😄 |
format! |
f8145d0
to
65d4ac9
Compare
format! |
9f9cab4
to
1f8e583
Compare
4438972
to
87dbd50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except for the core/matrix/csr, other LGTM
* | ||
* @return the number of stored elements per batch item. | ||
*/ | ||
size_type get_num_elements_per_item() const noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use get_num_stored_elements()
in other place.
should it be get_num_stored_elements_per_item()
for consistence? it's quite long though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think get_num_elements_per_item
is clear here. With stored
it gets too long. Maybe it can also be renamed to get_nnz_per_item
to make it shorter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we try to avoid nnz because the stored element can be zeros
* Csr is a general sparse matrix format that stores the column indices for each | ||
* nonzero entry and a cumulative sum of the number of nonzeros in each row. It | ||
* is one of the most popular sparse matrix formats due to its versatility and | ||
* ability to store a wide range of sparsity patterns in an efficient fashion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not update documentation with batch like batchEll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be updated. Or are you seeing something here that is incorrect ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's like CSR not batchCSR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note below clarifies that this is a batched matrix fomat, and how the batches are stored. I think being within the namespace also makes that clear. It is similar to batch::Ell
. But I think can add more clarification to make it clear.
auto max_num_elems = this->get_common_size()[0] * | ||
this->get_common_size()[1] * | ||
this->get_num_batch_items(); | ||
GKO_ASSERT(values_.get_size() <= max_num_elems); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to check this? it is just unused from the batch csr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, the get_num_stored_elements is based on values.size(), so it is required to only store the batch value
99d94ec
to
5c1bc41
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1450 +/- ##
===========================================
+ Coverage 89.33% 89.37% +0.04%
===========================================
Files 688 696 +8
Lines 56555 56944 +389
===========================================
+ Hits 50524 50895 +371
- Misses 6031 6049 +18 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Aditya Kashi <[email protected]>
Co-authored-by: Aditya Kashi <[email protected]>
Co-authored-by: Aditya Kashi <[email protected]>
Co-authored-by: Aditya Kashi <[email protected]>
Co-authored-by: Phuong Nguyen <[email protected]>
Co-authored-by: Pratik Nayak <[email protected]>
Co-authored-by: Marcel Koch <[email protected]>
Co-authored-by: Pratik Nayak <[email protected]>
Co-authored-by: Yu-Hsiang Tsai <[email protected]>
5c1bc41
to
6adf1a6
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs 68.6% Coverage The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think the testing strategy should be revisited at some point. A lot of tests in core test functionality that is composed of smaller parts, but the implementation is the same for all different matrix types. For example, batch::write
only requires that the type has create_const_view_for_item
. Because of that many tests are repetetive and could probably be unified.
As the previous pipeline had fully passed, and I only removed a couple of tests, I will just go ahead and merge this PR, not waiting for the whole pipeline to complete. |
This PR adds a batched Csr matrix format, that stores the same sparsity pattern for all entries, but different values for each batch. Only simple and advanced apply to
batch::MultiVector
are supported for now.TODO